opt: fix computation of apply join outer columns #35171

justinj · 2019-02-23T17:51:46Z

Previously, we would incorrectly compute the outer columns of an
apply-join, because we would not consider that some of the right input's
outer columns actually came from higher up in the tree, not from the
left input.

By fixing this, a new problem was uncovered that required fixing. The
following series of events led to an infinite loop of norm rules
applying:

An outer join requests null rejection on some set of columns,
that request travels up through an apply join,
it eventually reaches a GroupBy that can provide null rejection via
RejectNullsGroupBy,
RejectNullsGroupBy creates a new filter and pushes it down,
this new filter is unable to make it past the apply join that the
request traveled through,
RejectNullsGroupBy triggers again, since the null rejection request
persists.

The fix was to stop null rejection requests if the join doing the
propagation wouldn't be able to push down a filter (that is, if the
input has outer cols).

Unfortunately, we can't (easily) fix this by allowing filters to travel
through apply joins (via PushFilterIntoJoinRight), since that then
cycles with TryDecorrelateSelect (which is effectively the inverse of
that).

This also prevents decorrelation of an existing test case, which
requires further investigation.

Release note: None

cockroach-teamcity · 2019-02-23T17:51:54Z

This change is

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jordanlewis, @justinj, @RaduBerinde, and @rytaft)

pkg/sql/opt/memo/logical_props_builder.go, line 410 at r1 (raw file):

		// right input.
		rightOuterCols := join.Child(1).(RelExpr).Relational().OuterCols
		boundOuterCols := rightOuterCols.Intersection(join.Child(0).(RelExpr).Relational().OutputCols)

We already did DifferenceWith(inputCols) above which includes all the left child OutputCols.. So maybe we can remove this entire block?

pkg/sql/opt/norm/testdata/rules/reject_nulls, line 620 at r1 (raw file):

# them.
exprnorm
(Root

[nit] I don't think you need the Root part as long as you don't execbuild it.

rytaft

Reviewed 6 of 6 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jordanlewis, and @justinj)

pkg/sql/opt/testutils/opttester/opt_tester.go, line 591 at r1 (raw file):

		}
		ot.seenRules.Add(int(ruleName))
		return true

This seems like it's doing full exploration -- not just normalization (I think you want to do what OptNorm is doing above).

Previously, we would incorrectly compute the outer columns of an apply-join, because we would not consider that some of the right input's outer columns actually came from higher up in the tree, not from the left input. By fixing this, a new problem was uncovered that required fixing. The following series of events led to an infinite loop of norm rules applying: * An outer join requests null rejection on some set of columns, * that request travels up through an apply join, * it eventually reaches a GroupBy that can provide null rejection via RejectNullsGroupBy, * RejectNullsGroupBy creates a new filter and pushes it down, * this new filter is unable to make it past the apply join that the request traveled through, * RejectNullsGroupBy triggers again, since the null rejection request persists. The fix was to stop null rejection requests if the join doing the propagation wouldn't be able to push down a filter (that is, if the input has outer cols). Unfortunately, we can't (easily) fix this by allowing filters to travel through apply joins (via PushFilterIntoJoinRight), since that then cycles with TryDecorrelateSelect (which is effectively the inverse of that). This also prevents decorrelation of an existing test case, which requires further investigation. Release note: None

justinj

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jordanlewis, @RaduBerinde, and @rytaft)

pkg/sql/opt/memo/logical_props_builder.go, line 410 at r1 (raw file):

Previously, RaduBerinde wrote…

We already did DifferenceWith(inputCols) above which includes all the left child OutputCols.. So maybe we can remove this entire block?

Oh, you're totally right! Deleted it.

pkg/sql/opt/norm/testdata/rules/reject_nulls, line 620 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] I don't think you need the Root part as long as you don't execbuild it.

Done.

pkg/sql/opt/testutils/opttester/opt_tester.go, line 591 at r1 (raw file):

Previously, rytaft wrote…

This seems like it's doing full exploration -- not just normalization (I think you want to do what OptNorm is doing above).

It's not actually, exprgen.Build doesn't run exploration over the built expression. Added a comment explaining.

rytaft

Reviewed 3 of 3 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @jordanlewis, and @RaduBerinde)

pkg/sql/opt/testutils/opttester/opt_tester.go, line 591 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

It's not actually, exprgen.Build doesn't run exploration over the built expression. Added a comment explaining.

Cool - thanks!

andy-kimball

Seems we need to bring back cycle detection. I removed it thinking the Rule props were a good enough tool to prevent cycles, but this is the second known case where that unable to decorrelate due to not having the cycle detection.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @jordanlewis, and @RaduBerinde)

justinj · 2019-02-26T16:52:33Z

bors r+

35171: opt: fix computation of apply join outer columns r=justinj a=justinj Previously, we would incorrectly compute the outer columns of an apply-join, because we would not consider that some of the right input's outer columns actually came from higher up in the tree, not from the left input. By fixing this, a new problem was uncovered that required fixing. The following series of events led to an infinite loop of norm rules applying: * An outer join requests null rejection on some set of columns, * that request travels up through an apply join, * it eventually reaches a GroupBy that can provide null rejection via RejectNullsGroupBy, * RejectNullsGroupBy creates a new filter and pushes it down, * this new filter is unable to make it past the apply join that the request traveled through, * RejectNullsGroupBy triggers again, since the null rejection request persists. The fix was to stop null rejection requests if the join doing the propagation wouldn't be able to push down a filter (that is, if the input has outer cols). Unfortunately, we can't (easily) fix this by allowing filters to travel through apply joins (via PushFilterIntoJoinRight), since that then cycles with TryDecorrelateSelect (which is effectively the inverse of that). This also prevents decorrelation of an existing test case, which requires further investigation. Release note: None Co-authored-by: Justin Jaffray <[email protected]>

craig · 2019-02-26T17:40:46Z

Build succeeded

GitHub CI (Cockroach)

justinj requested review from jordanlewis, rytaft, RaduBerinde and andy-kimball February 23, 2019 17:51

justinj requested a review from a team as a code owner February 23, 2019 17:51

justinj force-pushed the apply-outer-cols branch 2 times, most recently from 999ea8f to 88bbdf7 Compare February 23, 2019 21:51

jordanlewis mentioned this pull request Feb 25, 2019

sql: implement apply join #34546

Merged

justinj force-pushed the apply-outer-cols branch from 88bbdf7 to bafd909 Compare February 25, 2019 15:36

RaduBerinde reviewed Feb 25, 2019

View reviewed changes

rytaft requested changes Feb 25, 2019

View reviewed changes

justinj force-pushed the apply-outer-cols branch from bafd909 to 0c183cd Compare February 25, 2019 19:42

justinj commented Feb 25, 2019

View reviewed changes

rytaft approved these changes Feb 25, 2019

View reviewed changes

andy-kimball approved these changes Feb 26, 2019

View reviewed changes

craig bot merged commit 0c183cd into cockroachdb:master Feb 26, 2019

justinj mentioned this pull request Mar 27, 2019

opt: infinite loop between normalization rules #36137

Closed

DrewKimball mentioned this pull request Jul 7, 2022

opt: simplify outer joins in more cases #83962

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: fix computation of apply join outer columns #35171

opt: fix computation of apply join outer columns #35171

justinj commented Feb 23, 2019 •

edited

Loading

cockroach-teamcity commented Feb 23, 2019

RaduBerinde left a comment

rytaft left a comment

justinj left a comment

rytaft left a comment

andy-kimball left a comment

justinj commented Feb 26, 2019

craig bot commented Feb 26, 2019

opt: fix computation of apply join outer columns #35171

opt: fix computation of apply join outer columns #35171

Conversation

justinj commented Feb 23, 2019 • edited Loading

cockroach-teamcity commented Feb 23, 2019

RaduBerinde left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

justinj left a comment

Choose a reason for hiding this comment

rytaft left a comment

Choose a reason for hiding this comment

andy-kimball left a comment

Choose a reason for hiding this comment

justinj commented Feb 26, 2019

craig bot commented Feb 26, 2019

Build succeeded

justinj commented Feb 23, 2019 •

edited

Loading